-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Getpid shim #2215
Getpid shim #2215
Conversation
2a6b1ec
to
1958475
Compare
I was not able to test and verify if the windows function work properly, this happens when I try to :
I tried giving multiple different ABIs to |
393337a
to
f5bc58c
Compare
This is a good start, thanks. :) I left some comments. What this is also missing is a test. The test should probably just call |
This seems ready to me, is there anything else? |
Please also add a test that runs on all platforms and just calls |
I couldn't find where to create windows' test, libc.rs seems to be reserved for unixy OSs |
Also winapi does not seem to be a dependency of miri, so I wonder if I'm supposed to create the test into test/pass/....rs |
Just create a file that calls |
This is weird, I'm having this as an error while trying what you told me :
meanwhile my code is : // compile-flags: -Zmiri-isolation-error=warn-nobacktrace
fn getpid() -> u32 {
std::process::id()
}
fn main() {
getpid();
} Oh right, std::process::id() calls std::sys::unix::os::getpid(), which itself calls libc::getpid(), therefore, what in the world |
The flag needs to be
|
1fe7ae8
to
def51a7
Compare
I've tried to cleanup a bit the mess that this tree is, let me know if you need me to squash further |
8e27fa1
to
7aea83a
Compare
This LGTM but please squash it all into a single commit. The last commit doesn't even change anything.^^ |
Oh, that failure is funny. It only occurs with optimizations enabled. Does inlining here "work around" the |
Yeah this is the full error:
Notice the "inside I guess so far we have been lucky that MIR inlining didn't do this for other calls. Can we check the current span instead, somehow? That should be stable under inlining (as the backtrace shows). |
make frame_in_std check work with inlining `@InfRandomness` this should help with your trouble in #2215
The issue didn't reproduce in the latest CI jobs, |
It only reproduced on windows. And I fixed it in #2225. :) |
Thanks! |
📌 Commit 3e03054 has been approved by |
Oh, I thought I had to apply that fix in here somewhere, |
☀️ Test successful - checks-actions |
…03, r=RalfJung Remove GetCurrentProcessId's frame_in_std check Most of the support required to close #1727 was actually added a while back, in #2215. However, for some reason, even though the Unix/Linux syscall equivalent has no `frame_in_std()` check, the Windows `GetCurrentProcessId` check did. While the vast majority of use cases use `std::process::id`, there's no particular reason to penalize any Windows code that is no_std or for whatever other reason choses to call the function directly (e.g. via the generated [windows-sys](https://docs.rs/windows-sys/latest/windows_sys/Win32/System/Threading/fn.GetCurrentProcessId.html) method). The emulation should still work fine. Given there's no reason not to, we might as well simplify the code a tiny bit and save that branch / frame check during runtime too. This PR removes the `frame_in_std` restriction for `GetCurrentProcessId`, and also moves it into the environment related shim section per discussion in #1727 (comment). Still passes existing tests/pass/getpid.rs test. Closes #1727 unless we wish to give a dummy value when isolated, which we don't seem to want to do at this time.
No description provided.